Skip to content

fix(ui5-multi-combobox): correct Select All with grouped items and n-more popover#13721

Open
Vonahz wants to merge 8 commits into
mainfrom
mcmbbx_selectall
Open

fix(ui5-multi-combobox): correct Select All with grouped items and n-more popover#13721
Vonahz wants to merge 8 commits into
mainfrom
mcmbbx_selectall

Conversation

@Vonahz

@Vonahz Vonahz commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fixes: #13712

@Vonahz Vonahz temporarily deployed to netlify-preview June 24, 2026 06:05 — with GitHub Actions Inactive
@Vonahz Vonahz temporarily deployed to netlify-preview June 24, 2026 06:06 — with GitHub Actions Inactive
@sap-ui5-webcomponents-release

Copy link
Copy Markdown

@Vonahz Vonahz temporarily deployed to netlify-preview June 30, 2026 10:49 — with GitHub Actions Inactive
@Vonahz Vonahz temporarily deployed to netlify-preview June 30, 2026 13:34 — with GitHub Actions Inactive
@Vonahz Vonahz temporarily deployed to netlify-preview June 30, 2026 14:33 — with GitHub Actions Inactive
@Vonahz Vonahz temporarily deployed to netlify-preview June 30, 2026 14:51 — with GitHub Actions Inactive
@Vonahz Vonahz temporarily deployed to netlify-preview June 30, 2026 18:34 — with GitHub Actions Inactive
@ivoplashkov ivoplashkov requested a review from nikoletavnv July 2, 2026 07:37
});

// Hide unselected items and empty groups using CSS
allItems.forEach(item => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need this code that modifies the css property?

return item.selected;
});

this._filteredItems = [...filtered];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove lines from 1684 to 1698 and replace them with:

this._filteredItems = allItems.filter(item => (item.isGroupItem ? item._isVisible : item.selected));

item._isVisible = visibleItems ? visibleItems.includes(item) : true;
}
// Reset display style
(item as HTMLElement).style.display = "";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line because the _applySelectedItemsFilter will no longer change the item style.display

.find("[ui5-token]")
.should("have.length", 3);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some tests related to Select All when you have the n-more popover opened

}

// Restore autocomplete on next tick to allow normal typing
setTimeout(() => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case we need this setTimeout ? Can we remove it and also remove the line at 1170?

* Used when selections change while the n-more popup is open.
* @private
*/
_updateGroupsVisibility() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can go without this method _updateGroupsVisibility and all its calls. Can you please check if it works without it for the scenario it was added.

@nikoletavnv

Copy link
Copy Markdown
Contributor

Design questions:

  1. Should we delete the input text once we select the filtered items through selectAll? In the past we deleted it, but if we select a single item - we keep it. Once we focus out from the multicombobox - the value is lost
  2. On mobile "Select all" is always visible in the dialog in contrast to the desktop where if the filter returns no items the popover is not shown at all and Select All is not displayed. Should we hide it (Select All ( 0 from 0 )) on mobile too?
  3. On mobile - click on the input to open the items list in a dialog. Then focus the input, press Tab to focus the SelectAll. The focus goes to a div container. Press Arrow Down to navigate through the list items - the focus again stays on the Select All but on the inner container. This is because ui5-mcb-select-all-header has tabindex 0. On desktop the header also has tabindex 0, but the focus skips it and the ui5-checkbox item for Select All is focused

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ui5-multi-combobox]: Select All is not checked when items are displayed in groups

4 participants